-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Limit number of stored errors #97
Conversation
Looks good. This makes sense, though I think in the case that we reach the limit, we should probably discard the oldest errors instead of the newest ones. I'm not sure on what is a reasonable limit though. |
Changed to delete the oldest error and store the new one. 100 seems like a reasonable number - if you have that many errors it is most likely the same thing repeatedly so there is probably limited value to reporting additional instances anyway. |
Awesome! 👍 |
How much space do 100 errors take up? Where are they stored by default? |
@d4rken The files are stored in the cache directory. |
The size will depend on lots of things like how big/deep the stack traces are and what custom meta-data is being added, but each error could end up being 10s of KB each at least. Storing 100 is not going to take up a huge amount of space but seems sensible to be conservative about the number of errors to store. Happy to change it if anyone has a different opinion though! |
Seems reasonable as default number. |
Limit number of stored errors
I think code comments don't show anymore when the PR is merged? Are my concerns valid or did i miss something? I must admit I'm not 100% versed in how the offline storage works at the moment. 7f0a626#commitcomment-15562085 7f0a626#commitcomment-15562415 If these are valid i would make an issue ticket and when I find time a PR. |
@d4rken I thought I'd reply here for visibility:
It ultimately uses the default sort from File.compareTo() which should order alphabetically. As the files are named with a timestamp, the smallest value (oldest time) is always at index 0.
I considered that we may end up with more than 100 if delete fails, but as it's an arbitrary limit I don't think it matters too much (as you suggest). |
Ok 👍 |
Android Changelog ==== ## 3.4.0 (2016-03-09) ### Enhancements - Limit the number of stored errors [Duncan Hewett](https://github.com/duncanhewett) [#97](bugsnag/bugsnag-android#97) ### Bug Fixes - Fix `ConcurrentModificationException` which could occur when saving breadcrumbs [Duncan Hewett](https://github.com/duncanhewett) [#98](bugsnag/bugsnag-android#98) - Localize all numbers in error metrics [Delisa Mason](https://github.com/kattrali) [#100](bugsnag/bugsnag-android#100) 3.3.0 (2016-01-18) ----- ### Enhancements - Change distribution method to be .aar only [Lars Grefer](https://github.com/larsgrefer) [#91](bugsnag/bugsnag-android#91) - Skip sending empty device data values [Matthias Urhahn](https://github.com/d4rken) [#96](bugsnag/bugsnag-android#96) - Remove the need for synthetic methods [Jake Wharton](https://github.com/JakeWharton) [#87](bugsnag/bugsnag-android#87) 3.2.7 (2015-12-10) ----- ### Enhancements - Add additional check to ensure the cache of uploaded errors are deleted [#80](bugsnag/bugsnag-android#80) ### Bug Fixes - Fix exception which occurs when `appContext.getResources()` is null [#78](bugsnag/bugsnag-android#78) - Fix bug preventing `maxBreadcrumbs` from being set [David Wu](https://github.com/wuman) [#70](bugsnag/bugsnag-android#70) 3.2.6 ----- - Add blocking API - Fix NPE issue - Concurrent adding to tabs - Thread Safe DateUtils#toISO8601
Limit the number of errors stored for later sending to prevent possible disk space issues (see #23).
Stores up to 100 errors and discards any subsequent errors. First 100 errors are stored on the basis that they are likely to be the most interesting. Is this sensible?